Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

reentrancy fix #9

Open
wants to merge 4 commits into
base: fuse-reactive-audit
Choose a base branch
from
Open

reentrancy fix #9

wants to merge 4 commits into from

Conversation

sriyantra
Copy link
Collaborator

No description provided.

@@ -135,8 +135,7 @@ contract CEther is CToken, CEtherInterface {

function doTransferOut(address payable to, uint amount) internal {
// Send the Ether and revert on failure
(bool success, ) = to.call.value(amount)("");
require(success, "doTransferOut failed");
to.transfer(amount);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is certain a fix but it would block all proxied contract from withdrawing ETH because they would revert on transfer call due to implied gas limit. to.call.value(amount) was introduced in the first place to address this issue.
It is also good idea to check how many proxied contacts have ETH positions on Rari, i.e. how many of them would get stuck.
One possible solution is outline here:
https://twitter.com/ylv_io/status/1520569475438813184

(bool success, ) = to.call.gas(10000).value(amount)("");

They it is not 100% bulletproof and has tradeoffs as well.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggest to use openzepplin's address library's sendValue to fix this problem.
/**
* @dev Replacement for Solidity's transfer: sends amount wei to
* recipient, forwarding all available gas and reverting on errors.
*
* https://eips.ethereum.org/EIPS/eip-1884[EIP1884] increases the gas cost
* of certain opcodes, possibly making contracts go over the 2300 gas limit
* imposed by transfer, making them unable to receive funds via
* transfer. {sendValue} removes this limitation.
*
* https://diligence.consensys.net/posts/2019/09/stop-using-soliditys-transfer-now/[Learn more].
*
* IMPORTANT: because control is transferred to recipient, care must be
* taken to not create reentrancy vulnerabilities. Consider using
* {ReentrancyGuard} or the
* https://solidity.readthedocs.io/en/v0.5.11/security-considerations.html#use-the-checks-effects-interactions-pattern[checks-effects-interactions pattern].
*/
function sendValue(address payable recipient, uint256 amount) internal {
require(address(this).balance >= amount, "Address: insufficient balance");

    (bool success, ) = recipient.call{value: amount}("");
    require(success, "Address: unable to send value, recipient may have reverted");
}

Comment on lines +706 to +709
/* We write previously calculated values into storage */
totalSupply = vars.totalSupplyNew;
accountTokens[redeemer] = vars.accountTokensNew;

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice. Also, consider using ReentrancyGuard. It will increase gas cost and have upgradability implications since it is using a storage variable.

@@ -803,6 +803,11 @@ contract CToken is CTokenInterface, Exponential, TokenErrorReporter {
// EFFECTS & INTERACTIONS
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As suggested by Igor, and considering the specific attack pattern suffered where an exitMarket at comptroller was triggered from a borrowing function at CToken, if possible I would recommend to explicitly disallow the reentrancy between those 2 contracts/functions at modifier-function level independently on the final gas limitation and/or check-effects-interaction pattern which are also good practices.

This is important because a small change in the logic in the future could bring back the issue so it is always better to explicitly block the call before entering an unexpected function in such workflow, as well as for code readability.

So I would not allow explicitly an exitMarket while in a borrow.

I would also suggest to use some kind of flashloan protection > 1 sec unless explicitly allowed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants